fix(a2a): accept top-level `token` on PushNotificationConfig (closes #61) by mabry1985 · Pull Request #64 · protoLabsAI/quinn

mabry1985 · 2026-04-17T05:30:36Z

Actual root cause of Quinn #61

Visibility fix in #62 surfaced the real error: Workstacean's callback endpoint was returning HTTP 401 "Invalid notification token" on every Quinn webhook delivery. Not 404, not silence — 401.

Why

The A2A spec `PushNotificationConfig` allows two interchangeable shapes for the bearer token:

Top-level `token` — what `@a2a-js/sdk` serialises by default. Workstacean's SkillDispatcherPlugin calls `setTaskPushNotificationConfig({ pushNotificationConfig: { url, token } })` (`a2a-executor.ts:329-331`).
Structured `authentication.credentials` — RFC-8821 `AuthenticationInfo` with `schemes` + `credentials`.

Quinn only read shape #2. Workstacean's top-level token fell on the floor → `cfg.token = None` → `_deliver_webhook` skipped the `Authorization: Bearer ` header → Workstacean's callback handler (`a2a-callback.ts:47-50`) found no bearer, computed `providedToken = ""`, mismatched, returned 401.

Live evidence (post-#62 logs)

```
push config registered (jsonrpc) task=1167ad7f... → .../callback/1167ad7f...
webhook delivered task=1167ad7f... state=completed → .../callback/1167ad7f... (401)
```

Fix

Factored token extraction into `_extract_push_token(cfg)` — checks top-level first, falls back to `authentication.credentials`, returns None when neither is present. All three PushNotificationConfig parse sites (JSON-RPC set handler, REST alias, shared `_parse_push_config` at submit time) now route through it. No more divergent parsing across entry points.

Closes

Closes #61

Test plan

4 regression tests for `_extract_push_token`: top-level, structured, both-present precedence, none-present → None
`pytest tests/ --ignore=tests/test_e2e_smoke.py` — 185 passed
Post-deploy: Workstacean → Quinn dispatch. Expected `webhook delivered ... (200)` instead of `(401)`, and the fleet push-notification table shows Quinn ✅ via webhook, not polling.

🤖 Generated with Claude Code

Summary by CodeRabbit

Bug Fixes
- Fixed push notification configuration handling to properly recognize token values specified at the top level, while maintaining backward compatibility with nested credential formats.
- Prioritized top-level token configuration when both formats are present.
Tests
- Added comprehensive unit tests for push notification token extraction logic.

…61) Real root cause of push-notification failures against Workstacean. Visibility fix in #62 surfaced the actual error — Workstacean's callback endpoint was returning HTTP 401 "Invalid notification token", not the 404/silence we initially feared. The A2A spec's PushNotificationConfig allows two interchangeable shapes for the webhook bearer token: 1. Top-level `token` — the simple form @a2a-js/sdk serialises by default. Workstacean's SkillDispatcherPlugin uses this path: `setTaskPushNotificationConfig({ pushNotificationConfig: {url, token} })` (a2a-executor.ts:329-331). 2. Structured `authentication.credentials` — RFC-8821 AuthenticationInfo with `schemes` + `credentials`. Quinn only read shape #2. Workstacean's top-level token fell on the floor → stored `cfg.token = None` → `_deliver_webhook` skipped the `Authorization: Bearer <token>` header → Workstacean's callback handler found no bearer, computed `providedToken = ""`, mismatched the expected token, and rejected with 401 for every delivery. Live evidence from v0.1.8 logs: push config registered (jsonrpc) task=1167ad7f... → .../callback/1167ad7f... webhook delivered task=1167ad7f... state=completed → .../callback/1167ad7f... (401) Fix: factored token extraction into `_extract_push_token(cfg)` that checks top-level first, falls back to `authentication.credentials`, and returns None cleanly when neither is present. All three PushNotificationConfig parse sites (JSON-RPC set handler, REST alias handler, shared `_parse_push_config` at submit time) now route through it — no more divergent token parsing across the three entry points. 4 regression tests cover: top-level token, structured authentication, both-present-precedence, none-present-is-None. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

coderabbitai · 2026-04-17T05:30:51Z

Walkthrough

This PR introduces a helper function _extract_push_token() to standardize webhook bearer token resolution from PushNotificationConfig inputs, prioritizing top-level token values while falling back to nested authentication.credentials. All config parsing paths updated to use this helper with comprehensive unit test coverage.

Changes

Cohort / File(s)	Summary
Token extraction helper `a2a_handler.py`	Added `_extract_push_token()` helper to resolve webhook bearer tokens with precedence: top-level `token` → `authentication.credentials` → `None`. Updated `_parse_push_config()`, `tasks/pushNotificationConfig/set` JSON-RPC path, and `_create_push_config()` REST path to use the helper, eliminating duplicate token extraction logic.
Unit tests `tests/test_a2a_handler.py`	Added four tests for `_extract_push_token()` covering: top-level `token` support, `authentication.credentials` fallback, token precedence when both present, and `None` return for missing/empty tokens.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 70.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately and concisely describes the main change: accepting top-level `token` on PushNotificationConfig, with clear issue reference.
Description check	✅ Passed	The description fully complies with the template structure, including Summary, Closes (`#61`), and comprehensive Test plan documentation.
Linked Issues check	✅ Passed	The PR addresses the root cause of issue `#61` by implementing consistent token extraction across all three PushNotificationConfig parse sites, enabling proper Authorization header population for webhook delivery.
Out of Scope Changes check	✅ Passed	All changes are narrowly scoped to token extraction logic and its tests; no unrelated modifications introduced outside of the stated objectives.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/push-config-accept-top-level-token

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@a2a_handler.py`:
- Around line 653-658: The code assumes cfg.get("authentication") returns a dict
and calls auth.get(...), which throws AttributeError for truthy non-dict values;
change the auth handling to validate its type before using .get (e.g., retrieve
auth = cfg.get("authentication"); if not isinstance(auth, dict): auth = {}),
then read creds = auth.get("credentials") and return creds only if it's a
non-empty string (as with the existing creds check). This targets the auth
variable and creds extraction in the shown function to avoid 500s on malformed
config.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 6efdb91d-dc3e-4891-922f-bcdb3e287384

📥 Commits

Reviewing files that changed from the base of the PR and between 86d50bf and 07ab624.

📒 Files selected for processing (2)

a2a_handler.py
tests/test_a2a_handler.py

coderabbitai · 2026-04-17T05:33:24Z

+    top_level = cfg.get("token")
+    if isinstance(top_level, str) and top_level:
+        return top_level
+    auth = cfg.get("authentication") or {}
+    creds = auth.get("credentials")
+    return creds if isinstance(creds, str) and creds else None


⚠️ Potential issue | 🔴 Critical

Handle non-dict authentication to avoid 500s on malformed config.

Line 656 assumes authentication is a dict. If a client sends a truthy non-object value, auth.get(...) raises AttributeError and the request fails with 500 instead of safely resolving to None.

🔧 Proposed fix

def _extract_push_token(cfg: dict) -> str | None: @@ - auth = cfg.get("authentication") or {} - creds = auth.get("credentials") + auth = cfg.get("authentication") + if not isinstance(auth, dict): + return None + creds = auth.get("credentials") return creds if isinstance(creds, str) and creds else None

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed. In `@a2a_handler.py` around lines 653 - 658, The code assumes cfg.get("authentication") returns a dict and calls auth.get(...), which throws AttributeError for truthy non-dict values; change the auth handling to validate its type before using .get (e.g., retrieve auth = cfg.get("authentication"); if not isinstance(auth, dict): auth = {}), then read creds = auth.get("credentials") and return creds only if it's a non-empty string (as with the existing creds check). This targets the auth variable and creds extraction in the shown function to avoid 500s on malformed config.

Follow-up to #64 / #62. Two operator-visible tweaks in the streaming + push notifications section: - Call out that the token-parsing accepts both spec shapes (top-level `token` used by @a2a-js/sdk + Workstacean, and structured `authentication.credentials` for RFC-8821-style callers). - Document `LOG_LEVEL=INFO` (the default) as the switch that surfaces every push-config registration + webhook delivery attempt. Paired with workstacean docs that call out these as gold-standard requirements for every a2a agent.

Follow-up to #64 / #62. Two operator-visible tweaks in the streaming + push notifications section: - Call out that the token-parsing accepts both spec shapes (top-level `token` used by @a2a-js/sdk + Workstacean, and structured `authentication.credentials` for RFC-8821-style callers). - Document `LOG_LEVEL=INFO` (the default) as the switch that surfaces every push-config registration + webhook delivery attempt. Paired with workstacean docs that call out these as gold-standard requirements for every a2a agent. Co-authored-by: GitHub CI <ci@example.com>

coderabbitai Bot reviewed Apr 17, 2026

View reviewed changes

mabry1985 merged commit 01eb31e into main Apr 17, 2026
1 check passed

mabry1985 deleted the fix/push-config-accept-top-level-token branch April 17, 2026 05:39

This was referenced Apr 17, 2026

docs(README): note dual token-shape support + LOG_LEVEL tuning #66

Merged

docs(a2a): spell out dual-shape token parsing + INFO logging protoLabsAI/protoWorkstacean#412

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(a2a): accept top-level `token` on PushNotificationConfig (closes #61)#64

fix(a2a): accept top-level `token` on PushNotificationConfig (closes #61)#64
mabry1985 merged 1 commit intomainfrom
fix/push-config-accept-top-level-token

mabry1985 commented Apr 17, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Apr 17, 2026 •

edited

Loading

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

coderabbitai Bot Apr 17, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

mabry1985 commented Apr 17, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Actual root cause of Quinn #61

Why

Live evidence (post-#62 logs)

Fix

Closes

Test plan

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

mabry1985 commented Apr 17, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Apr 17, 2026 •

edited

Loading